refactor(ng-dev/ts-circular-dependencies): reorder processing of warnings and golden existence check #2611
Conversation
…ings and golden existence check Move the warnings processing before the initial processing of golden file and approval to always include its output. Additionally, provide alternate text output for approval attempts when no golden exists.
| Log.warn(` Please rerun with "--warnings" to inspect unresolved imports.`); | ||
| } | ||
|
|
||
| if (goldenFile) { |
There was a problem hiding this comment.
NIt: I'd prefer if we guard by approve at the first condition, as that would make it easier to follow IMO.
(maybe that's just me though)
There was a problem hiding this comment.
I think that going based on the golden makes sense because its essentially shortcutting the rest of the logic below when no golden exists.
I reordered some things that I think end sup making it a bit better, let me know what you think.
…of warnings and golden existence check
…of warnings and golden existence check
| ); | ||
| return 1; | ||
| } | ||
| if (cycles.length > 0) { |
There was a problem hiding this comment.
Shouldn't this be outside? What if there is a goldenFile and cycles?
There was a problem hiding this comment.
This is within the goldenFile being undefined so there is no golden file.
There was a problem hiding this comment.
But shouldn't this fail? if there are cycles and goldenFile being !== undefined?
There was a problem hiding this comment.
I guess it will happen automatically below with the comparison. That's the thing I missed. Very confusing :D
There was a problem hiding this comment.
For the case where there is no golden file, and there are cycles here. This fails as it returns a 1 as the exit code.
For cases where there is a golden file to do the comparison to later, we check the specifics.
| return 0; | ||
| } | ||
|
|
||
| if (!existsSync(goldenFile)) { |
There was a problem hiding this comment.
Wouldn't this always exist after writing before?
There was a problem hiding this comment.
The file would only be guaranteed to be written to, if an approval happened. If its a regular check, then it wouldn't be written in the block above, and could technically not exist. We verify it exists because just below here we read from it to do the actual comparison.
| ); | ||
| return 1; | ||
| } | ||
| if (cycles.length > 0) { |
There was a problem hiding this comment.
I guess it will happen automatically below with the comparison. That's the thing I missed. Very confusing :D
|
This PR was merged into the repository by commit 0ad6a37. The changes were merged into the following branches: main |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
2 participants
Move the warnings processing before the initial processing of golden file and approval to always include its output. Additionally, provide alternate text output for approval attempts when no golden exists.